Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Uptime] Use dynamic index pattern in Uptime #55446

Merged
merged 11 commits into from
Jan 30, 2020
Merged

[Uptime] Use dynamic index pattern in Uptime #55446

merged 11 commits into from
Jan 30, 2020

Conversation

shahzad31
Copy link
Contributor

@shahzad31 shahzad31 commented Jan 21, 2020 •
edited
Loading

Summary

Fix: #53616

Use Index Pattern service to create fake Index pattern.

Will only fetch index pattern once unlike current implementation.

Testing:

  1. make sure kuery bar is working as expected in Uptime

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@shahzad31 shahzad31 self-assigned this Jan 21, 2020
@shahzad31 shahzad31 added release_note:fix Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.6.0 v7.7.0 v8.0.0 labels Jan 21, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

andrewvc
andrewvc requested changes Jan 22, 2020
View reviewed changes
Copy link
Contributor

@andrewvc andrewvc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably shouldn't merge this because we probably shouldn't be creating an index pattern in the first place.

I did some digging, we only use the index pattern data for the Kuery Bar, but nothing about it requires it to be an actual index pattern saved object. We should instead just directly serve the JSON from a REST endpoint as a file based resource since it's shipped with Kibana. We never really needed to use saved objects here in the first place.

I'll add this note to the issue.

We should probably close this PR since it doesn't fix the root cause.

@shahzad31
Copy link
Contributor Author

We probably shouldn't merge this because we probably shouldn't be creating an index pattern in the first place.

I did some digging, we only use the index pattern data for the Kuery Bar, but nothing about it requires it to be an actual index pattern saved object. We should instead just directly serve the JSON from a REST endpoint as a file based resource since it's shipped with Kibana. We never really needed to use saved objects here in the first place.

I'll add this note to the issue.

We should probably close this PR since it doesn't fix the root cause.

@andrewvc problem with Json file is that it is hard to maintain. If we add a field in mapping, we will have to manually update the file. Generating Index Pattern from Mapping and using that seems like a good approach to me.

@shahzad31 shahzad31 changed the title [Uptime] Index pattern creation using auto attribute field and fixed title [Uptime] Use dynamic index pattern in Uptime Jan 24, 2020
@shahzad31
Copy link
Contributor Author

@elasticmachine merge upstream

andrewvc
andrewvc requested changes Jan 28, 2020
View reviewed changes
Copy link
Contributor

@andrewvc andrewvc left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionality is so much better, and it's so much cleaner with Redux!

x-pack/legacy/plugins/uptime/server/rest_api/index_pattern/get_index_pattern.ts Outdated Show resolved Hide resolved
x-pack/legacy/plugins/uptime/common/constants/constants.ts Outdated Show resolved Hide resolved
x-pack/legacy/plugins/uptime/public/hooks/update_kuery_string.ts Outdated Show resolved Hide resolved
x-pack/legacy/plugins/uptime/public/hooks/update_kuery_string.ts Outdated Show resolved Hide resolved
x-pack/legacy/plugins/uptime/public/hooks/update_kuery_string.ts Outdated Show resolved Hide resolved
x-pack/legacy/plugins/uptime/public/hooks/update_kuery_string.ts
const filterMap = new Map<string, Array<string | number>>(JSON.parse(urlFilters));
kueryString = stringifyKueries(filterMap);
}
} catch {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize that this is not new code, but it may be worth addressing while we're working on this.

In general swallowing errors is something we should avoid because it makes debugging issues (esp. in production) very difficult. Tracing back a genuine error to this code would be tricky, and then we wouldn't know if the issue was in the stringify part, the map instantiation, or the JSON parsing. If we have to do it we should limit it to a single statement. If it's to catch the JSON parse failure then we should do something like:

let parsed;
let parseError;
try {
  parsed = JSON.parse(urlFilters);
} catch (e) {
  parseError = e;
}

// We can safely ignore parse errors here because <<insert explanation>>
if (parseError) {
  kueryString = stringify(kueries)
} else {
  ...the happy path...
}

Then, we could execute conditional logic later depending on whether there's an error or not.

Note that we should document the reason why we're swallowing the error here. Is there a situation where the JSON is expected to be invalid? If so, why? Why is it better to ignore it rather than surfacing an error?

My suspicion is that we can possibly even remove the error handling here. @justinkambic may know more here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we can't have control over user input into url or kuery bar, i have improve the code to make it more readable.

x-pack/legacy/plugins/uptime/public/hooks/update_kuery_string.ts Show resolved Hide resolved
x-pack/legacy/plugins/uptime/server/lib/adapters/saved_objects/kibana_saved_objects_adapter.ts Outdated Show resolved Hide resolved
@shahzad31
Copy link
Contributor Author

@andrewvc i took care of PR feedback, regarding those try/catch, i think we need them, since user can enter any value into url, so it will be hard to predict, if it will parse.

andrewvc
andrewvc approved these changes Jan 30, 2020
View reviewed changes
Copy link
Contributor

@andrewvc andrewvc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I still have some concerns around our error handling, but since this isn't new code I'm fine merging this. I'll open a follow-up PR with reworked error handling since that might be a better way to show what I'm talking about.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@shahzad31 shahzad31 merged commit a8622cf into elastic:master Jan 30, 2020
@shahzad31 shahzad31 deleted the fix/auto-generated-index-pattern-uptime branch January 30, 2020 11:10
This was referenced Jan 30, 2020
Merged
Merged
shahzad31 added a commit to shahzad31/kibana that referenced this pull request Jan 30, 2020
@shahzad31 @elasticmachine
[Uptime] Use dynamic index pattern in Uptime (elastic#55446)
22977c4 
* Using auto attribute field and fixed title

* added constant

* refactor index pattern state

* fixed type

* PR feedback

* resolve conflcits

Co-authored-by: Elastic Machine <[email protected]>
shahzad31 added a commit to shahzad31/kibana that referenced this pull request Jan 30, 2020
@shahzad31 @elasticmachine
[Uptime] Use dynamic index pattern in Uptime (elastic#55446)
19f3c19 
* Using auto attribute field and fixed title

* added constant

* refactor index pattern state

* fixed type

* PR feedback

* resolve conflcits

Co-authored-by: Elastic Machine <[email protected]>
shahzad31 added a commit that referenced this pull request Jan 30, 2020
@shahzad31 @elasticmachine
[Uptime] Use dynamic index pattern in Uptime (#55446) (#56386)
e2fc958 
* Using auto attribute field and fixed title

* added constant

* refactor index pattern state

* fixed type

* PR feedback

* resolve conflcits

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
shahzad31 added a commit that referenced this pull request Jan 30, 2020
@shahzad31 @elasticmachine
[7.6] [Uptime] Use dynamic index pattern in Uptime (#55446) (#56387)
896470f 
* [Uptime] Use dynamic index pattern in Uptime (#55446)

* Using auto attribute field and fixed title

* added constant

* refactor index pattern state

* fixed type

* PR feedback

* resolve conflcits

Co-authored-by: Elastic Machine <[email protected]>

* fix issue with filter update

Co-authored-by: Elastic Machine <[email protected]>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jan 31, 2020
@jloleysens
Merge branch '7.x' of github.com:elastic/kibana into backport/7.x/pr-…
f8afb1b 
…56356

* '7.x' of github.com:elastic/kibana: (23 commits)
  Fix setting filters without $store value (elastic#56304) (elastic#56475)
  [ML] Fix Data Visualizer responsive layout  (elastic#56372) (elastic#56472)
  [ML] conditional rison encoding for query params (elastic#56380) (elastic#56469)
  kuery_autocomplete -> convert remaining items to TS/Jest (elastic#56316) (elastic#56471)
  [APM] Fit service map to container (elastic#56336) (elastic#56463)
  Add animation to service map layout (elastic#56042) (elastic#56460)
  chore(NA): delete data/optimize with kbn clean (elastic#55890) (elastic#56422)
  [APM] Storybook support (elastic#54970) (elastic#56445)
  [DOCS] Updates example in Timelion doc (elastic#56444) (elastic#56454)
  [Logs UI] Fix Check for New Data button on empty indices screen (elastic#56239) (elastic#56320)
  [DOCS] Adds breaking changes for 7.6 (elastic#56437)
  [Monitoring] Change all configs to `monitoring.*` (elastic#56215) (elastic#56421)
  [skip-ci] Add example for migrating pre-handlers (elastic#56080) (elastic#56436)
  [7.x] System index templates can't be edited (elastic#55229) (elastic#56417)
  Add missing docker settings (elastic#56411)
  [Uptime] Use dynamic index pattern in Uptime (elastic#55446) (elastic#56386)
  fix edit rule for detections (elastic#56333) (elastic#56405)
  [Filter Bar] Remove flickering when opening filter bar popover (elastic#56222) (elastic#56385)
  [ILM] Index Lifecycle Policies show wrong unit in Kibana UI (elastic#55228) (elastic#55757)
  Move tsvb server to new platform (elastic#55310) (elastic#56394)
  ...
@shahzad31 shahzad31 mentioned this pull request Jan 31, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewvc andrewvc andrewvc approved these changes

@justinkambic justinkambic Awaiting requested review from justinkambic

Assignees

@shahzad31 shahzad31

Labels
release_note:fix Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.6.0 v7.7.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Uptime] Auto-generated index pattern doesn't work with discover
4 participants
@shahzad31 @elasticmachine @kibanamachine @andrewvc